test(amazon Q): fix test failure caused by MockTelemetryExtension throwing NotAMockException #5258
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Will-ShaoHua
changed the title
Will-ShaoHua
changed the title
rli
reviewed
Jan 14, 2025
| <applicationService serviceInterface="migration.software.aws.toolkits.jetbrains.services.telemetry.TelemetryService" | ||
| serviceImplementation="software.aws.toolkits.jetbrains.services.telemetry.DefaultTelemetryService" | ||
| testServiceImplementation="software.aws.toolkits.jetbrains.services.telemetry.NoOpTelemetryService"/> | ||
| serviceImplementation="software.aws.toolkits.jetbrains.services.telemetry.DefaultTelemetryService"/> |
Contributor
There was a problem hiding this comment.
can we keep it as no-op so that tests dont send metrics by default, but you can add the rule if you want to assert metrics?
Contributor
Author
There was a problem hiding this comment.
aha that makes sense
rli
reviewed
Jan 14, 2025
| private val mockTelemetryService: NoOpTelemetryService by lazy { NoOpTelemetryService(publisher, batcher) } | ||
|
|
||
| override fun before() { | ||
| ApplicationManager.getApplication().replaceService(TelemetryService::class.java, mockTelemetryService, mockTelemetryService) |
Contributor
There was a problem hiding this comment.
replaceService needs to be reverted in the after()
Will-ShaoHua
commented
Jan 16, 2025
|
|
||
| override fun after() { | ||
| reset(batcher()) | ||
| Disposer.dispose(disposableParent) |
Contributor
Author
There was a problem hiding this comment.
@rli is it sufficient for the purpose of reverting replaceService?
Will-ShaoHua
changed the title
rli
approved these changes
Jan 17, 2025
karanA-aws
pushed a commit
to karanA-aws/aws-toolkit-jetbrains
that referenced
this pull request
Jan 22, 2025
…owing NotAMockException (aws#5258) ## Description <!--- Describe your changes in detail --> <!--- If appropriate, providing screenshots will help us review your contribution --> <!--- If there is a related issue, please provide a link here --> Now there are 2 ways to use NoOpTelemetryService 1. if devs are not interested in validating the metric contents, don't need do anything as default test implementation is NoOpTelemetryService 2. if devs are interested in the metric contents, can use `MockTelemetryServiceExtension` which will inject a spy of batcher and replace the original test implementation in junit before hook and revert it in after hook so that you can use ArgumentCaptor to capture what's going through the batcher Relevant aws#5207 ### root cause: Originally by doing `class NoOpTelemetryService : TelemetryService(publisher, spy(DefaultTelemetryBatcher(publisher))) {`, it's not 100% guaranteed that the TelemetryBatcher passed in is a mockito spy, and it result in `NotAMockException` thrown from these cases. ### solution Manually inject spy batcher separately in a different step instead of IDE doing the work when initializing app/project service components--------- Co-authored-by: Richard Li <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Now there are 2 ways to use NoOpTelemetryService
MockTelemetryServiceExtensionwhich will inject a spy of batcher and replace the original test implementation in junit before hook and revert it in after hook so that you can use ArgumentCaptor to capture what's going through the batcherRelevant #5207
root cause:
Originally by doing
class NoOpTelemetryService : TelemetryService(publisher, spy(DefaultTelemetryBatcher(publisher))) {, it's not 100% guaranteed that the TelemetryBatcher passed in is a mockito spy, and it result inNotAMockExceptionthrown from these cases.solution
Manually inject spy batcher separately in a different step instead of IDE doing the work when initializing app/project service components
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.